Restore the AgentProcess a thread already held, rather than clearing it - #1912

Two review questions answered: a step-by-step scenario, and virtual threads. The second one narrows the scope of this PR, so I've corrected the description above.

Does this affect virtual threads? No — and not the platform default either

threading.shared defaults to false, so AsyncConfiguration builds Embabel's own executor:

• virtual threads on → Executors.newThreadPerTaskExecutor(...) — always starts a new thread
• platform threads (the default) → Executors.newCachedThreadPool(...) — always hands off to a worker

Neither can run a task on the thread that submitted it, so neither was ever affected. The clearing this PR replaces was harmless on both.

Two tests now pin that (OwnedExecutorsNeverRunOnTheCaller), one per executor, each asserting the task genuinely ran on a different thread before asserting the caller kept its process. Both pass before this PR's fix as well as after — that is the claim, so they are only meaningful if verified against unfixed main, which they were.

So reaching the defect requires two things together: embabel.agent.platform.threading.shared=true, AND an application executor that can run a task on the submitting thread — CallerRunsPolicy with a bounded queue, or a direct/synchronous executor. Out of the box, nobody hits this. My original description said "appears under load and not in development" without that precondition, which overstated it.

It is still worth fixing: sharing the app's executor is a supported, documented option, and a propagation primitive that quietly empties a thread local is the kind of thing that costs somebody a day.

The scenario, step by step

Setup: threading.shared=true, and the application's executor has a bounded queue with CallerRunsPolicy. The system is under enough load that the queue is full.

• Time 1 — A request arrives on HTTP thread http-7. Agent process P starts. On http-7, AgentProcess.get() returns P.
• Time 2 — An action runs. A guardrail writes an object onto P's blackboard. Still http-7. Works.
• Time 3 — Platform code calls asyncer.async { ... } from http-7 — a parallel tool call, a sub-process, anything going through Asyncer.
• Time 4 — Every worker is busy and the queue is full. CallerRunsPolicy therefore runs the task on http-7 itself. This is the only unusual step, and it only happens when saturated.
• Time 5 — ExecutorAsyncer captured P from the calling thread, and now sets P on the "worker" thread. The worker thread is http-7, which already had P. Nothing visibly changes.
• Time 6 — The task body runs and sees P. Correct so far.
• Time 7 — The task finishes. The finally calls reset(), which removes the value instead of putting back what was there. http-7 now has no process. Nothing throws.
• Time 8 — Control returns to the action, still on http-7, still inside the same request, still believing it is inside P.
• Time 9 — An interceptor further along calls AgentProcess.get() to read the blackboard. It gets null.
• Time 10 — It reports the object missing, or NPEs, or silently skips.

The damage is done at Time 7 and shows up at Time 9, on a different code path, with nothing logged in between. And because Time 4 only happens when the pool is saturated, the same request succeeds whenever a worker happens to be free — so it is intermittent and load-dependent.

With the fix, Time 7 restores P instead of clearing it, and Time 9 sees P.

What is unchanged

PooledThreadIsolation asserts the property the clearing existed for, and it passes before and after: a pooled worker never carries one task's process into the next, two processes never bleed across thread reuse, and parallelMap leaves nothing behind. The fix is "restore instead of clear", not "stop cleaning up".

@igordayen — right, EmbabelObjectMapperHolder.kt was not mine. A git add -A swept up a license header the Maven license plugin had written into that file while the build ran. Nothing to do with this change. Removed: force-pushed a rebuilt branch, now three files, all of them ones this PR actually touches.

(That file does still lack its header on main — a leftover from #1887. Trivial, but it belongs in its own change, not smuggled in here.)

Also: the scenario and the virtual-threads answer are in the comment above, and the timeline is now in the issue as well, as you asked.

@alexheifetz — no objection to holding it until after 1.5.0; sequencing is your call. Two things that may be useful in deciding.

Scope is narrower than it first reads. With the default configuration this is unreachable. threading.shared is false by default, so the platform builds its own executor — a cached pool, or thread-per-task on virtual threads — and neither can run a task on the submitting thread. It needs threading.shared=true plus an application executor that can (CallerRunsPolicy with a bounded queue, or a direct executor), and then only while that pool is saturated. It is also not a regression: the behaviour has been there since #1477.

On testing. Agreed this is the part that has to be right, so the suite is built to be falsifiable rather than confirmatory:

• The tests were written against unfixed main first. 5 of 12 fail there and pass after — the four direct-executor cases and the caller-runs one. The other 7 pass both before and after, and each was run against unfixed main to confirm that, so they are pinning existing behaviour rather than describing the patch.
• PooledThreadIsolation asserts the property the old clearing existed for — a worker never carrying one task's process into the next, no bleed across thread reuse, parallelMap leaving nothing behind. If the fix were "stop cleaning up", those go red.
• OwnedExecutorsNeverRunOnTheCaller covers virtual and cached-pool, each asserting the task genuinely ran on a different thread before asserting the outcome, so they cannot pass vacuously.
• CallerRunsUnderLoad asserts its precondition (the overflow task really did run on the submitting thread) before asserting the result, for the same reason.

Verification: new suite 12/12; 28/28 across the asyncer and accessor tests repeated 5x for flakiness, stable every run; embabel-agent-api 4019 tests, 0 failures; full reactor green.

If there is a specific scenario you want covered before it lands — an IT-level one, or a shape I have not thought of — name it and I will add it.

@jasperblues per my understanding scenario is the edge one, when all application threads are busy. I would enhance documentation with note: use two pools with caution, increase size as needed. For Assistant switch to virtual threads. Thanks
@alexheifetz FYI

@igordayen — but pool sizing changes how often the task lands on the submitting thread. It doesn't change what happens when it does: ExecutorAsyncer clears a thread local it didn't set, so the caller comes back from async() with no AgentProcess. A larger pool makes that rarer and no less silent.

Thinking about the end user: better that it never happens than that they think "I should have read the small print" after a production crash.

@igordayen — taking the docs note and the load test. Added SharedExecutorCallerRunsWiringIT, which runs with the IT suite per #1577 and needs no keys: it boots the real AsyncConfiguration with threading.shared=true against a bounded CallerRunsPolicy pool, saturates it, and asserts the overflow genuinely ran on the submitting thread before asserting the outcome. It fails on the unfixed code with expected: <AgentProcess> but was: <null> and passes with it. A second case pins that shared=false — the default — never reaches that shape at all.

On the extra guard — I think it is already there, structurally rather than as configuration. Restore differs from clear only when the calling thread already held a process, which is precisely the caller-runs case; on every other path it restores nothing and behaves exactly as before. A flag would add a configuration in which the defect is still reachable, and a predicate that can itself be wrong. PooledThreadIsolation pins the unchanged half.

@jasperblues @alexheifetz

Please consider adding Tests:

with restores previous process after success
with restores previous process after exception
with restores outer process after nested with
with clears process after block when previous process was null
with null value leaves current process unchanged, or explicitly test whatever behavior is intended

The last one matters because current code says:

if (value == null) {
return block()
}

So with(null) does not clear. That is okay if intentional, but the test should pin it.

I agree we should not add the shared-executor guard conditions.

Docs To Add In This PR
Add a short note to the threading model docs, not a long warning.

Location:

embabel-agent-docs/src/main/asciidoc/reference/asynch-mode/page.adoc

Place it near the threading.shared explanation.

Suggested text:

NOTE: embabel.agent.platform.threading.shared=true means Embabel may use the application's
applicationTaskExecutor when the application and Embabel use the same threading model. If that executor is
bounded and uses CallerRunsPolicy, saturated executor workers and a full queue can cause Embabel async work
to run on the submitting application thread. Embabel preserves AgentProcess context in this case, but this
configuration still couples agent execution to application executor backpressure and may affect request
latency. Use executor sharing deliberately.

Follow-Up After This PR (I can take it):
Create a separate issue for the config guard.

Bare minimum guard work later:

Add property:
embabel.agent.platform.threading.shared-executor-caller-runs-policy=warn

Values:
allow, warn, fail

Apply only when sharedExecutor != null.

Warn or fail for:
ThreadPoolExecutor + CallerRunsPolicy
Detect known caller-thread executors.
Warn that custom executors may also run tasks on caller threads and cannot always be detected.

Tests for that follow-up:

shared=true + CallerRunsPolicy + warn starts and logs warning
shared=true + CallerRunsPolicy + fail fails startup
shared=false + CallerRunsPolicy + fail starts because Embabel does not share it
shared=true + normal executor + fail starts

Please run all IT tests.
Thanks for the important discovery.

@igordayen — taken all of it. Tests, the docs note, and the follow-up left to you.

with tests

Added to AgentProcessAccessorTest, which already existed alongside ExecutorAsyncerContextPropagationTest and covered getValue/setValue/reset but nothing on with. Six tests, in a nested With class:

• restores the previous process after the block returns
• restores the previous process when the block throws
• restores the outer value after a nested with, not just the innermost — asserts at every level on the way out, not only the last
• clears the slot when the thread held nothing to begin with — the case clearing was written for
• a null value leaves a process the thread already holds in place
• a null value leaves an empty thread empty, and still runs the block

ExecutorAsyncerCallerThreadTest covers the same guarantee through the executor, which is where it is reached and why it matters. These state the contract on its own terms, so a change to with fails against its own rules rather than against the behaviour of a saturated thread pool several layers away.

with(null)

You are right, and it is intentional. A null value is not "clear the process for the duration" — it means the caller had nothing to propagate, which is what ExecutorAsyncer passes when a task is submitted from outside any process. The block then runs against whatever the running thread already holds, and with neither sets nor clears. Now pinned in both directions, and said on the method.

Mutation-checked rather than assumed. Reverting the finally to a bare reset() fails four of them; making with(null) clear fails the two null-value tests.

Docs

Your text verbatim, in reference/asynch-mode/page.adoc next to the threading.shared table, before the Asyncer paragraph.

Tests run

SharedExecutorCallerRunsWiringIT green (2/2), and the full suite green on this branch — 2852 tests, 0 failures.

Follow-up

Agreed on leaving the config guard out of this PR — take it, and the property shape you sketched reads right to me.

@Jasper, the only thing I would like to get clarity on is the use case.
If it is a shared bounded executor with CallerRunsPolicy, or virtual threads. Thanks

@igordayen — I think I have found the mismatch you meant, and you were right to flag it: the Testing section had gone stale. It described the branch as it stood on 11 Aug, before the two rounds of work you asked for. So the prose named one test class and 12 cases, while the branch had three test files and 20.

Nothing in it was false — the numbers were correct on the day they were written, and each round since simply added to the branch without the description being brought forward. Recording old vs new rather than quietly editing:

What was added after that snapshot, both at your request:

• SharedExecutorCallerRunsWiringIT (12 Aug) — the saturated CallerRunsPolicy pool through the real AsyncConfiguration, asserting the task genuinely ran on the submitting thread. Fails on unfixed code.
• AgentProcessAccessorTest.With (13 Aug) — 6 cases pinning the restore contract on with() directly, including both directions of with(null), which you specifically asked to have pinned.
• The asynch-mode docs note, your text.

Description now matches the branch, with the superseded figures noted rather than erased.

The rest of the prose I checked while I was in there, and it holds:

• "not a regression, the behaviour has been there since AgentProcess propagation to worker threads #1477" — confirmed by git log -S'AgentProcessAccessor.reset()', which lands on 0c0fbe182.
• "five of the twelve fail on unfixed main" — re-confirmed by mutation this morning: 4 in DirectExecutor, 1 in CallerRunsUnderLoad.
• The virtual-threads paragraph — corrected yesterday after your earlier note.

If the mismatch you meant was something else, point me at it and I will fix that too.

@igordayen — added four more cases, all of which fail on unfixed main. Failure count goes from 5/12 to 9/16. Pushed as a205e508f.

The gap they close is parallelMap, which is how an action fans out and is where this defect looks least like a bug. Every item goes through async(), so on an executor that can run on the submitter, the first item empties the thread and later ones run with no process — one call, partly correct results, nothing thrown anywhere.

Unfixed:

• direct executor → fails at item 2
• saturated CallerRunsPolicy pool → fails at item 4

Both parallelMap branches are covered, since maxConcurrency < size takes the separate semaphore path. A single-async test cannot express erosion, and erosion is the shape a real deployment meets.

The fourth states the symptom rather than the mechanism: after an async on the caller's thread, is the blackboard still reachable? Unfixed, that read returns null. The suite now describes the failure in the words someone would use to report it — "the blackboard lost my object" — rather than only in terms of a thread local.

Description updated: 24 cases across three files, 40/40 on the asyncer and accessor suites.

@igordayen — three more cases, pushed as d31c62880. Executor-level coverage that fails on unfixed main goes from 9 to 13 of 21 (11 of 18 in ExecutorAsyncerCallerThreadTest, 2 of 3 in the wiring IT).

The IT gained the primitive actions actually fan out with. It drove a single async through the real AsyncConfiguration, which proves a deployment can reach the caller thread — but not what happens when it gets there via parallelMap, which is the path OperationContext.parallelMap and parallel actions take. Sixteen items over a one-worker pool, so some are certain to run on the submitter. Fails on unfixed code.

A failing item in a fan-out. One tool call of several throwing is ordinary, not exotic. Unfixed, the caller comes out of it holding no process — so the error handling that runs next is the code that discovers the blackboard is empty. Failure and loss of context arriving together is the worst version of this bug, because the exception looks like the whole story.

A task that sets its own process. This one pins the direction of the restore rather than its presence. Putting back what the caller had is not the same as leaving whatever the task last set, and a test asserting only "not null" would pass on an implementation that handed the task's process upward — work attributed to, and billed against, the wrong process.

Counts taken from the surefire XML rather than the summary lines, since reruns make those misleading. Description updated: 27 cases across three files, 43/43 green.

@igordayen — answering "how do you want to handle this PR" with something checkable rather than more prose.

#1930 carries the tests from this PR and nothing else — no change to ExecutorAsyncer, none to AgentProcessAccessor — applied to current main. Draft, and not for merge.

• Tests alone on main: 13 of 21 fail.
• Restore the AgentProcess a thread already held, rather than clearing it #1912 merged on top: 31 of 31 pass.

Both steps are two commands, in the description. Roughly two minutes, no keys needed.

The 8 that pass in step 1 matter as much as the 13 that fail. OwnedExecutorsNeverRunOnTheCaller is green before and after — that is the evidence for "virtual threads cannot hit this", rather than my assertion of it. PooledThreadIsolation is green before and after too, which is the evidence that the fix is not just "stop cleaning up". If all 21 went red, the suite would be proving too much to be trusted.

One thing deliberately left out of #1930: AgentProcessAccessorTest.With calls AgentProcessAccessor.with(), which this PR introduces, so on main it would fail to compile. A compile error is the wrong kind of red — it demonstrates nothing about behaviour. Everything in #1930 compiles against main and fails at runtime.

On handling: no urgency from me. #1894 is merged and I am unblocked, so this can sit as long as you like. If the reproduction is what was missing, #1930 is it; if the prose is still the problem, point at the line and I will fix or cut it.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

A few minor observations to consider:

1. @ param on with() — add a note that null means "run block without modifying the slot," since that behavior is non-obvious. (AgentProcessAccessor.kt)
2. Comment on else reset() — one line explaining "pool thread arrived empty; clear so the next task doesn't inherit." The asymmetry is correct, but for a reader, it is not an intuitive context. (AgentProcessAccessor.kt:69)
3. Mixed mocking libraries — AgentProcessContextPropagationTest uses Mockito, ExecutorAsyncerCallerThreadTest uses MockK. If the project has a standard, worth aligning.

No functional changes needed.
Please execute IT tests. thanks

@jasperblues - critical IT tests in embabel-agent-experimental/embabel-experimental-integration-tests passed.